Skip to content

feat(core): add portable document ingestion contracts - #1178

Merged
phernandez merged 7 commits into
mainfrom
codex-document-ingestion
Aug 29, 2026
Merged

feat(core): add portable document ingestion contracts#1178
phernandez merged 7 commits into
mainfrom
codex-document-ingestion

Conversation

@phernandez

@phernandez phernandez commented Aug 1, 2026

Copy link
Copy Markdown
Member

Why

Hosted document ingestion needs one portable Basic Memory contract for generated Markdown, provenance, deterministic identity, and safe semantic parsing. Without that boundary, Core and Cloud would disagree about what a raw extraction note means, how retries converge, and which fields an enrichment agent may control.

This PR is the Core contract used by basic-memory-cloud#1650, implements the parser-neutral portion of #1006, and deliberately leaves historical revision indexing to #1177.

Specification

Scope

Core must provide strict, provider-neutral contracts for:

  • a current generated Markdown note with type: document;
  • a compact, queryable provenance note with type: document_ingestion_run;
  • deterministic document and run-note identity derived from trusted inputs;
  • canonical raw Markdown whose body is searchable but cannot mint observations or relations from arbitrary extracted syntax;
  • a bounded structured enrichment output that cannot replace trusted source, extraction, ingestion, checksum, or identity fields.

Behavioral contract

  1. source, extraction, ingestion, and document metadata is strict, nested, extra-forbidden, and serializable through either the YAML schema alias or the Python schema_ref field name.
  2. Document and run identities are deterministic. Run identity includes source identity/checksum, extractor engine/version/profile/options, pipeline version, and prompt version.
  3. Raw Markdown uses bm_parse_semantics: false; Core preserves body content for search while suppressing observation/relation parsing only for an explicit scalar false.
  4. Run-note stage, output revision kind, output identity, and output path must agree with the trusted inputs.
  5. Agent-provided body, tags, observations, and relations must round-trip through the real Markdown parser without hidden or additional semantics.
  6. Enrichment starts from an exact raw canonical checksum and reconstructs the final trusted envelope server-side.
  7. Public schema imports must not pull the heavy Markdown stack into lightweight CLI registration.

Non-goals

  • selecting or running a PDF parser;
  • Cloud queues, Tigris reads/writes, provider version IDs, or deployment policy;
  • enabling production agent enrichment;
  • historical revision storage, search, graph reconstruction, retention, or legal hold.

Acceptance Criteria

  • document and document_ingestion_run have strict versioned Pydantic contracts.
  • Nested provenance frontmatter is canonical, queryable, and round-trips through model serialization.
  • Document, run, and sidecar paths are derived from stable trusted inputs.
  • OCR page diagnostics reject coerced booleans, floats, and strings.
  • Raw generated content remains body-searchable while arbitrary PDF syntax cannot create graph semantics.
  • Run lifecycle state cannot contradict its raw/current output revision.
  • Agent output cannot smuggle extra tags, observations, relations, context, or trusted provenance.
  • Exact raw checksum compare-and-swap is required before enrichment assembly.
  • Lightweight CLI imports remain lazy.
  • The portable contract is exported for the Cloud implementation without adding a parser/runtime dependency.

What Changed

  • Added the document, extraction, ingestion-run, revision, and agent-output models in src/basic_memory/schemas/document.py.
  • Added deterministic UUID/path helpers, canonical Markdown parsing/assembly, SHA-256 helpers, lifecycle validation, and protected enrichment assembly.
  • Exported the portable contracts from src/basic_memory/schemas/__init__.py.
  • Added explicit document_ingestion note-object source support.
  • Updated EntityParser to honor the raw semantic opt-out safely without dropping body content.
  • Added focused schema, Markdown, runtime, and CLI-import regression coverage.

Implementation Details

  • UUID5 run identity is recomputed from the source and pipeline fields instead of trusting a supplied UUID.
  • Run-note output identity/path is recomputed from the source entity and source path.
  • Canonical Markdown reconstruction protects system-owned metadata and validates the fully assembled semantic body, including fence/comment boundary cases.
  • Agent tags use a canonical single-tag grammar; structured observations and relations must parse back to exactly the submitted objects.
  • Markdown parser imports remain inside the validators that need them, preserving the lightweight CLI boundary.

Testing

Automated

  • just fast-check: passed Ruff fix/check, formatting, and ty type checking.
  • uv run pytest -q tests/schemas/test_document.py tests/markdown tests/test_runtime.py::TestRuntimeContracts::test_note_object_metadata_parses_safe_values_only: 128 passed; src/basic_memory/schemas/document.py reached 100% statement coverage.
  • uv run pytest -q --no-cov tests/cli/test_cli_exit.py::test_bm_cli_import_does_not_load_heavy_stack: 1 passed.
  • GitHub CI on 61d7b0dd55313076a081bad823eb69796d4b8426: unit, SQLite integration, Postgres integration, semantic, Milvus, static, CodeQL, DCO, and CLA jobs passed or were skipped as designed.
  • git diff --check: passed.

Manual

  • No UI or manual runtime verification applies to this parser-neutral contract PR.

Risks / Follow-ups

  • Cloud must continue to validate only the trusted shapes it actually produces; additional future enrichment/history shapes can tighten their own invariants when implemented.
  • Agent enrichment remains disabled until Cloud can prove exact raw Tigris materialization (basic-memory-cloud#1649).
  • Current search and graph behavior remains head-only; the opt-in “BM data lake” is #1177.
  • The commit is SSH-signed and DCO-signed-off. GitHub reports unknown_key, so this description does not claim GitHub Verified status.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e5810e4dca

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/basic_memory/schemas/document.py
Comment thread src/basic_memory/schemas/document.py
Comment thread src/basic_memory/schemas/document.py
Comment thread src/basic_memory/schemas/document.py
Comment thread src/basic_memory/markdown/entity_parser.py Outdated
@phernandez
phernandez force-pushed the codex-document-ingestion branch from e5810e4 to 5a0ad61 Compare August 1, 2026 04:00

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5a0ad615fe

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/basic_memory/schemas/document.py
Comment thread src/basic_memory/schemas/document.py
@phernandez phernandez changed the title Add portable document ingestion contracts feat(core): add portable document ingestion contracts Aug 1, 2026
@phernandez phernandez added enhancement New feature or request cloud Basic Memory Cloud labels Aug 1, 2026
@phernandez
phernandez force-pushed the codex-document-ingestion branch from 5a0ad61 to cbd903c Compare August 1, 2026 04:43

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cbd903c254

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/basic_memory/schemas/document.py
Comment thread src/basic_memory/schemas/document.py
@phernandez
phernandez force-pushed the codex-document-ingestion branch from cbd903c to fca36b5 Compare August 1, 2026 05:34

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fca36b555a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/basic_memory/schemas/document.py
@phernandez

Copy link
Copy Markdown
Member Author

@codex review

@phernandez
phernandez force-pushed the codex-document-ingestion branch from fca36b5 to fa57ffe Compare August 1, 2026 05:47

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fa57ffe030

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/basic_memory/schemas/document.py Outdated
@phernandez

Copy link
Copy Markdown
Member Author

@codex review

@phernandez
phernandez force-pushed the codex-document-ingestion branch from fa57ffe to bc13b3d Compare August 1, 2026 06:17
@phernandez

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bc13b3d444

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/basic_memory/schemas/document.py
Comment thread src/basic_memory/schemas/document.py Outdated
@phernandez
phernandez force-pushed the codex-document-ingestion branch from bc13b3d to 61d7b0d Compare August 1, 2026 06:35

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 61d7b0dd55

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/basic_memory/schemas/document.py
Comment thread src/basic_memory/schemas/document.py
Comment thread src/basic_memory/schemas/document.py
@phernandez

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 61d7b0dd55

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/basic_memory/schemas/document.py
Comment thread src/basic_memory/schemas/document.py
@phernandez

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. More of your lovely PRs please.

Reviewed commit: 61d7b0dd55

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 87b60bde48

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/basic_memory/schemas/document.py
@calebjpicker

calebjpicker commented Aug 2, 2026

Copy link
Copy Markdown

Following this contract with interest — the bm_parse_semantics: false decision especially, keeping arbitrary extracted syntax from minting observations. That seems exactly right.

One question before DocumentAgentObservationV1 freezes at v1, because I want to see if you think it's feasible whether this idea can be extended in the manner described here.

#135 has been open since June 2025 asking for this at the note level — section and observation anchors, so a quote can cite its source precisely. Its first example was a [quote] observation linked to a specific figure in a specific section of another document.

That blocker looks smaller now than it did then. Compared to last year, observations now carry their own permalinks — search_service.py reads obs.permalink and de-duplicates on it, and each observation is its own search_index row with its own permalink and category. The addressable identifiers exist now; what's still missing is that relations target entities only.

#1178 is where it becomes load-bearing. DocumentAgentObservationV1 is category, content, tags, context — no locator. So an extracted quote will be traceable to a document, and to an exact stored version of it, which is awesome! But it still doesn't link to a place inside it. For legal and academic work that's the difference between a source and a cite — "Exhibit 12" versus "Exhibit 12 at 4:17–22." The extractor already knows pages: page_count, extracted_page_count, pages_needing_ocr. The information exists at extraction time and is dropped before it reaches an observation. An optional field now looks easier to implement now rather than a v2 migration later, though you'd know better than I would whether that's actually true.

I don't have the standing to say what fits your architecture, so I asked Claude to sketch options merely as suggestions. Flagging that explicitly — these are prompts, not recommendations, and I'd defer to your read on all of them of coures:

  1. Convention only. Put the cite in the context parenthetical that already exists — - [quote] "..." (p. 2). No schema change at all; buys a convention rather than anything resolvable.
  2. Optional structured locator on DocumentAgentObservationV1 — say an optional page number — rendered deterministically into context so the line still round-trips through the real parser and satisfies require_exact_parser_semantics.
  3. Page map on the document note. Store offset→page ranges once at extraction, so any later feature can resolve a span to a page without re-parsing the PDF.
  4. The general Section and Observation Anchor Wikilinks Support #135 fix. Let relations target observation permalinks, with a PDF page anchor as one instance of it.

My own thoughts--once parsed, can it store a page number and line number per page? That nested structure could potentially support direct quote architecture?

EDIT (claude helped me understand the constraints better): Related thought — if a span could carry the source checksum alongside its location, a citation becomes verifiable rather than just locatable: you could confirm a quote still matches the document it came from, and detect drift if the source is ever re-extracted. Given how much this contract already checksums, that might be closer to free than it sounds. - Claude

Entirely possible this belongs after v1 ships, or that one of these cuts against something I can't see from outside. Mostly I wanted to raise it while the contract is still open rather than after it isn't.

@phernandez phernandez added the On Hold Don't review or merge. Work is pending label Aug 28, 2026
phernandez and others added 3 commits August 29, 2026 00:02
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
The 'Simplify document semantic parsing' commit removed the
bm_parse_semantics opt-out that the PR #1178 specification promises,
so raw extraction bodies containing '- [category] ...', hashtags, or
[[wiki links]] were indexed as observations and relations straight
from untrusted PDF text. The unresolved Codex P1 on #1178 flagged
exactly this: test_raw_note_uses_normal_graph_semantics asserted the
polluting behavior as correct.

Restore the contract:
- DocumentNoteFrontmatterV1 requires bm_parse_semantics and rejects
  any value that disagrees with the ingestion stage (false for
  raw/failed, true for ready/needs_review).
- DocumentIngestionRunFrontmatterV1 pins bm_parse_semantics to false.
- enrich_document_markdown re-enables semantics only for the bounded
  enriched output.
- EntityParser honors the opt-out solely for an explicit scalar false
  (bool or normalized string), keeping body content for search and
  ignoring non-scalar values without crashing.

Replace test_raw_note_uses_normal_graph_semantics with the original
test_raw_search_only_note_does_not_mint_graph_semantics assertions,
restore the non-scalar safety test, and add a paired regression test
proving the assembled raw note parses with zero observations and
relations through the real EntityParser while the identical body
without the opt-out does mint them.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017STCpbNsYjZgUdftxgEAZ4
Signed-off-by: phernandez <paul@basicmachines.co>
@phernandez
phernandez force-pushed the codex-document-ingestion branch from 87b60bd to 5af61b3 Compare August 29, 2026 05:10
@phernandez

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-08-29T13:53:51.829888Z 971459f Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@phernandez

Copy link
Copy Markdown
Member Author

@calebjpicker — on the page/section locator for DocumentAgentObservationV1: not blocking v1, and here's the plan so it isn't a v2 migration either.

DocumentAgentObservationV1 stays as-is for this PR (category, content, tags, context). The v1 contract is deliberately the minimum trusted enrichment shape, and it's already versioned — the V1 suffix and the strict/extra-forbid models mean adding an optional locator later is additive, not a breaking migration. So freezing v1 now doesn't cost you the citation.

The reason it's not in this PR: a real locator wants the source page/line map to exist to resolve against, and that lives on the document/run provenance side (source/extraction already carry page_count, pages_needing_ocr), plus it ties into #135's ask for addressable observation anchors. That's its own slice with its own tests, not something to bolt onto the enrichment model under time pressure. When it lands it'll be an optional structured field (page, optional line/offset) that renders deterministically into context so the line still round-trips through the real parser — your option 2, which is the one that keeps the "verifiable cite" property you want (and the checksum angle you added is exactly why it's worth doing well rather than fast).

I'll open a follow-up issue for "optional source locator on document observations (page → context, resolve against the extraction page map)" and link #135 so it's tracked rather than lost. Thanks for pushing on this while the contract was still open.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5af61b359a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/basic_memory/schemas/document.py
Codex review on #1178: a source path like `C:/outside/report.pdf` (or
the drive-relative `C:x`) passes PurePosixPath's is_absolute() check,
but the derived sidecar stays drive-qualified and FileService joins it
with base_path via `base_path / entity.file_path`, discarding the
project root and materializing the note outside the project. Reject a
PureWindowsPath drive or rooted form as well, matching the note-move
path validation.

Refs #1178

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017STCpbNsYjZgUdftxgEAZ4
Signed-off-by: phernandez <paul@basicmachines.co>
@phernandez

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ac09a17e34

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/basic_memory/schemas/document.py Outdated
…ng them

Codex review on #1178: the source and output path fields used
NonEmptyText (strip_whitespace=True), so a path with leading/trailing
whitespace was silently rewritten by the model, while the public
derive_document_note_path validates the raw value and would reject it —
so the accepted provenance and derived sidecar could diverge and a
padded path could collide with another object's note. Use a
non-stripping path type and reject leading/trailing whitespace in the
canonical validator, matching AGENTS.md's fail-fast rule against silent
rewrites.

Refs #1178

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017STCpbNsYjZgUdftxgEAZ4
Signed-off-by: phernandez <paul@basicmachines.co>
@phernandez

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 34c4e9254d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/basic_memory/schemas/document.py
Comment thread src/basic_memory/schemas/document.py
…nt contracts

Two more Codex findings on #1178:
- The raw-extraction Markdown body accepted NUL bytes; assemble preserves
  them and the hosted path writes to Postgres note_content (TEXT), which
  rejects NUL before EntityParser's cleanup. Reject NUL at the body
  validation boundary.
- Source keys like `docs/CON.pdf` (reserved device name) or
  `docs/report.pdf:stream` (NTFS alternate data stream) passed path
  validation but never materialize as portable sidecars on Windows.
  Reject reserved device-name components and colon-based streams.

Refs #1178

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017STCpbNsYjZgUdftxgEAZ4
Signed-off-by: phernandez <paul@basicmachines.co>
@phernandez

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 14b1e63612

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/basic_memory/schemas/document.py
Codex review on #1178: beyond reserved names and colons, a POSIX source
path can still carry components Windows cannot represent — the illegal
characters <>:"|?* or a component ending in a dot or space (Windows
strips those). The derived sidecar can't materialize under that exact
name, so canonical note storage can't converge. Reject them in the
project-relative path validator.

Refs #1178

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017STCpbNsYjZgUdftxgEAZ4
Signed-off-by: phernandez <paul@basicmachines.co>
@phernandez

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 971459f036

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

def derive_document_note_path(source_file_path: str) -> str:
"""Return the portable sidecar path for one source file (for example ``report.pdf.md``)."""
source_path = _validate_project_relative_path(source_file_path)
return f"{source_path}.md"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject sidecar names that exceed filesystem limits

When the source's final component is a valid 253–255-byte filename on an ext4-backed project, this unconditional suffix produces a 256–258-byte sidecar name. Validation accepts the path, but materialization then fails permanently with ENAMETOOLONG, so the accepted document cannot converge to its portable file representation; validate the derived component length, including the .md suffix.

AGENTS.md reference: AGENTS.md:L156-L160

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining this one. It's technically real but out of scope for this PR and mis-scoped to the schema layer:

  • It requires a source filename within ~2 bytes of the OS maximum (253–255 bytes); no realistic source file is named that way.
  • Core's own note-write path (write_note → file_service) has no filename-length check either, so a long note title hits the identical ENAMETOOLONG. Adding a bespoke length guard to the document contract alone holds it to a stricter standard than the base note format it feeds into.
  • A hardcoded 255-byte check isn't portable: NAME_MAX is 255 bytes on ext4 but 255 UTF-16 units elsewhere and larger on other filesystems, and total-path limits differ. The only place the real limit is knowable is the file layer at write time.
  • It fails loudly (ENAMETOOLONG) rather than corrupting or converging to a wrong state; note_content is retained and still serves reads.

If we want NAME_MAX handling, it should be one shared guard in the file layer covering write_note and document ingestion together — tracked separately, not bolted onto this schema. Leaving this open would just add a non-portable check that the rest of the system doesn't enforce.

@phernandez
phernandez merged commit be634be into main Aug 29, 2026
31 checks passed
@phernandez
phernandez deleted the codex-document-ingestion branch August 29, 2026 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cloud Basic Memory Cloud enhancement New feature or request On Hold Don't review or merge. Work is pending

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants